-
Notifications
You must be signed in to change notification settings - Fork 801
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Flaky: TestSDKServerWatchGameServerFeatureSDKWatchSendOnExecute #1667
Flaky: TestSDKServerWatchGameServerFeatureSDKWatchSendOnExecute #1667
Conversation
pkg/sdkserver/sdkserver_test.go
Outdated
@@ -718,7 +718,7 @@ func TestSDKServerWatchGameServerFeatureSDKWatchSendOnExecute(t *testing.T) { | |||
} else { | |||
assert.Fail(t, "Channel is closed!") | |||
} | |||
default: | |||
case <-time.After(30 * time.Second): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this still need to try 2 times? If you are going to add a timeout, maybe remove the for statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is looking for two events to be passed through - the initial one on connection, then the second one from WatchGameServer.
I could rewrite it as two select
statements, but that feels a little verbose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't catch that part. I think it might be a bit clearer and/or more idiomatic to do a for { }
loop with a break (when it times out or when you've gotten the expected events) but this is ok too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would there be a concern that it would never escape the loop though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You would escape the loop as long as you add break / return (or panic) statements to each case, e.g.
for {
select {
case _, ok := <-stream.msgs:
// do stuff - maybe assert that the right messages show up?
break
case <-time.After(timeout):
// error, since we hit a timeout when we were expecting messages
t.Fail() // should cause the test to end, so no need to break / return
}
}
Build Succeeded 👏 Build Id: ca5520c2-d9c1-4826-875a-d341304f9fd0 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
@@ -700,6 +699,8 @@ func TestSDKServerWatchGameServerFeatureSDKWatchSendOnExecute(t *testing.T) { | |||
stop := make(chan struct{}) | |||
defer close(stop) | |||
sc.informerFactory.Start(stop) | |||
|
|||
fakeWatch.Add(fixture.DeepCopy()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a fake Watch so we can control explicitly when the game server is modified.
@@ -709,17 +710,31 @@ func TestSDKServerWatchGameServerFeatureSDKWatchSendOnExecute(t *testing.T) { | |||
assert.Nil(t, waitConnectedStreamCount(sc, 1)) | |||
assert.Equal(t, stream, sc.connectedStreams[0]) | |||
|
|||
// modify for 2nd event in watch stream | |||
fixture.Status.State = agonesv1.GameServerStateAllocated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roberthbailey @akremsa - I reworked this test to check for:
- We get what we expect in events on watch and modification
- That we don't get too many events
- We can't go into an infinite loop
PTAL, let me know what you think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice update! I like the way you've updated a select section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also suggested to use state other than Ready.
Switching default: case to a timeout, since due to the asynchronous nature of event processing, it could take a moment or two for the extra event to be processed. This is particularly true on the CI system, where there is a lot of processing going on, and it may take more time than usual to process.
cf7cf12
to
1ce642d
Compare
Build Succeeded 👏 Build Id: 33b634a5-675b-43c3-a112-3e0d56307c93 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: markmandel, roberthbailey The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…leforgames#1667) * Flaky: TestSDKServerWatchGameServerFeatureSDKWatchSendOnExecute Switching default: case to a timeout, since due to the asynchronous nature of event processing, it could take a moment or two for the extra event to be processed. This is particularly true on the CI system, where there is a lot of processing going on, and it may take more time than usual to process.
What type of PR is this?
/kind cleanup
What this PR does / Why we need it:
Switching default: case to a timeout, since due to the asynchronous nature of event processing, it could take a moment or two for the extra event to be processed.
This is particularly true on the CI system, where there is a lot of processing going on, and it may take more time than usual to process.
Which issue(s) this PR fixes:
Closes #
Special notes for your reviewer: